Repository navigation
Fix ClickHouseDataSource: OpenConnectionAsync() without a token returns ClickHouseConnection - #688
Open
polyglotAI-bot wants to merge 2 commits into
Open
polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
…ns ClickHouseConnection
ClickHouseDataSource.OpenConnectionAsync(CancellationToken) had no default
value for its token. A call with no argument did not bind to it and bound to
the base DbDataSource.OpenConnectionAsync(CancellationToken = default), which
is typed ValueTask<DbConnection>. The documented ADO.NET examples that then
call connection.CreateCommand("...") did not compile (CS1501).
Declare the token as `= default`, as IClickHouseDataSource already does, and
update the PublicAPI files.
Fixes: #686
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
October 15, 2026 07:31
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The source-breaking return-type change needs user-facing migration guidance beyond the PR description.
1 open finding
What changed in this PR
Fixes #686 in the HTTP client's ADO.NET API so awaiting ClickHouseDataSource.OpenConnectionAsync() returns ClickHouseConnection.
Changes:
- Makes the cancellation token optional.
- Updates API records and adds a regression test.
- Adds a changelog fragment.
| File | Description |
|---|---|
| ClickHouse.Driver/PublicAPI/PublicAPI.Unshipped.txt | Records the optional token. |
| ClickHouse.Driver/PublicAPI/net8.0/PublicAPI.Shipped.txt | Updates the framework-specific signature. |
| ClickHouse.Driver/ADO/ClickHouseDataSource.cs | Defaults the cancellation token. |
| ClickHouse.Driver.Tests/ADO/DataSourceTests.cs | Tests the no-argument call and command execution. |
| changelog.d/686-datasource-openconnectionasync.fixes.md | Announces the fix. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| IClickHouseConnection IClickHouseDataSource.OpenConnection() => OpenConnection(); | ||
|
|
||
| public new async Task<ClickHouseConnection> OpenConnectionAsync(CancellationToken cancellationToken) | ||
| public new async Task<ClickHouseConnection> OpenConnectionAsync(CancellationToken cancellationToken = default) |
Collaborator
Author
There was a problem hiding this comment.
Checked on this branch. The source change is real: ValueTask<DbConnection> t = ds.OpenConnectionAsync(); now fails with CS0029, and .AsTask() / .Preserve() on the call fail with CS1061. ((DbDataSource)ds).OpenConnectionAsync() still compiles and returns ValueTask<DbConnection>. DbConnection c = await ds.OpenConnectionAsync(); also still compiles.
- Changelog: done in ac7929b. The
686fragment now has a sub-bullet that names the new return type, the code that stops compiling, and the two ways to keep it compiling.changelog.cs --checkpasses. docs/http.mdx: no change. The docs page describes the current API, and the#ado-net-datasourceexamples are already correct for it. The page has no per-version compatibility notes; its only migration guide is for theClickHouse.Client→ClickHouse.Driverrename.CHANGELOG.mdis where this repo records source breaks for an upgrade (for example theLogger→LoggerFactoryentry). I left this thread open so a maintainer can decide whether they also want a docs note.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A no-argument call on ClickHouseDataSource now returns Task<ClickHouseConnection>, not ValueTask<DbConnection>. Code that stores the call as ValueTask<DbConnection>, or calls .AsTask()/.Preserve() on it, no longer compiles. Tell users how to keep compiling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #686 (solution 1, as the maintainer chose).
ClickHouseDataSource.OpenConnectionAsync(CancellationToken cancellationToken)had no default value for its token. So a call with no argument,await dataSource.OpenConnectionAsync(), did not bind to it. It bound to the baseDbDataSource.OpenConnectionAsync(CancellationToken = default), which is typedValueTask<DbConnection>. The documented ADO.NET examples indocs/http.mdxthen callconnection.CreateCommand("..."), which does not exist onDbConnection, and fail with CS1501. The fix declares the token asCancellationToken cancellationToken = default, asIClickHouseDataSource.OpenConnectionAsyncalready does. The no-argument call now binds to theClickHouseDataSourcemethod and is typedTask<ClickHouseConnection>.Changes
ClickHouse.Driver/ADO/ClickHouseDataSource.cs: add= defaultto the token ofOpenConnectionAsync.PublicAPI/PublicAPI.Unshipped.txtandPublicAPI/net8.0/PublicAPI.Shipped.txt: update the signature line (the only two files that list it).changelog.d/686-datasource-openconnectionasync.fixes.md: changelog fragment.docs/http.mdxsnippets are now correct as written.Test
DataSourceTests.OpenConnectionAsync_WithoutCancellationToken_ReturnsOpenClickHouseConnectionopens a connection with the no-argument call against the server. It assigns the result toClickHouseConnection, checks that the state isOpen, and runsCreateCommand("SELECT 1"). Without the fix the test project does not compile (CS0266: cannot convertDbConnectiontoClickHouseConnection). With the fix it passes.Also checked by hand against a 26.9 server: the three
docs/http.mdxsnippets from the issue (lines 1296, 1350 and 1556) now compile and run. These calls keep their previous types: an explicit token (Task<ClickHouseConnection>), a call throughIClickHouseDataSource(Task<IClickHouseConnection>), and a call through aDbDataSource-typed variable (ValueTask<DbConnection>).Compatibility
ClickHouseDataSource-typed variable is typedTask<ClickHouseConnection>, notValueTask<DbConnection>. Code that usesvar/awaitkeeps compiling. Code that stores the unawaited call in an explicitValueTask<DbConnection>, or calls.AsTask()/.Preserve()on it, stops compiling. Calls throughDbDataSource, throughIClickHouseDataSource, and method-group conversions do not change.Checklist
🤖 Generated with Claude Code